Skip to content

Add more metadata for pipelines#399

Open
MSattrtand wants to merge 26 commits intomainfrom
improve-pipeline-metadata
Open

Add more metadata for pipelines#399
MSattrtand wants to merge 26 commits intomainfrom
improve-pipeline-metadata

Conversation

@MSattrtand
Copy link
Collaborator

@MSattrtand MSattrtand commented Jul 3, 2025

@MSattrtand MSattrtand requested a review from blcham July 3, 2025 09:36
blcham

This comment was marked as outdated.

@MSattrtand MSattrtand force-pushed the improve-pipeline-metadata branch from 12cec5c to 0f355a6 Compare July 18, 2025 13:12
@blcham blcham force-pushed the improve-pipeline-metadata branch from 0f355a6 to cdd3d2d Compare July 18, 2025 15:29
Copy link
Contributor

@blcham blcham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comments

@blcham blcham self-requested a review July 23, 2025 07:45
Comment on lines +358 to +360
addProperty(moduleExecution, SPIPES.has_script, module.getResource().toString().replaceAll("\\/[^.]*$", ""));
if(!metadataMap.containsKey(SPIPES.has_script.toString())){
metadataMap.put(SPIPES.has_script.toString(), module.getResource().toString().replaceAll("\\/[^.]*$", ""));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
addProperty(moduleExecution, SPIPES.has_script, module.getResource().toString().replaceAll("\\/[^.]*$", ""));
if(!metadataMap.containsKey(SPIPES.has_script.toString())){
metadataMap.put(SPIPES.has_script.toString(), module.getResource().toString().replaceAll("\\/[^.]*$", ""));
String script = module.getResource().toString().replaceAll("\\/[^.]*$", "");
addProperty(moduleExecution, SPIPES.has_script, script);
if(!metadataMap.containsKey(SPIPES.has_script.toString())){
metadataMap.put(SPIPES.has_script.toString(), script);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate code but i still do not understand why we need to do this ... can you explain me whole this part what is doing, why we need it ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. It saves the script's name as a property to be added to the database, then saves it to metadataMap if it's not there. We need to have it there so we'll have access to it from any function in the logger. E.g. right now I have a code like:
private void persistPipelineExecutionStarted(final EntityManager em, long pipelineExecutionId, Thing pipelineExecution, final String functionName, final String scriptPath)
Where functionName and scriptPath are parameters of the function I've added. To avoid using them as inputs of the function, I should use metadataMap and put these parameters there. It will also allow for easy access to these parameters from any other function if needed.

@blcham blcham self-requested a review July 23, 2025 08:11
Copy link
Contributor

@blcham blcham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comments

@MSattrtand MSattrtand force-pushed the improve-pipeline-metadata branch from 3f011cc to adc41d3 Compare October 10, 2025 10:27
@MSattrtand MSattrtand force-pushed the improve-pipeline-metadata branch from 0412d55 to 516ae21 Compare October 17, 2025 12:54
@blcham blcham force-pushed the improve-pipeline-metadata branch from 516ae21 to b7be03e Compare November 14, 2025 15:23
} else {
scriptPath = module.getScriptPath();
}
String script = module.getResource().toString().replaceAll("\\/[^.]*$", "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what following is doing ..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored, so it would be more readable

addProperty(pipelineExecution, SPIPES.has_pipeline_execution_duration, computeDuration(startDate, finishDate));
addProperty(pipelineExecution, SPIPES.has_pipeline_name, pipelineName);
// addScript(pipelineExecution, scriptManager.getScriptByContextId(pipelineName));
pipelineExecution.setTypes(Collections.singleton(Vocabulary.s_c_finished_pipeline_execution));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we replace? ... not sure if we do not want to addTypes instead of setTypes ... multiple types should coexist

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, why we had to add em.getTransaction().commit() now? How about using addProperty(pipelineExecution, RDF_TYPE, Vocabulary.s_c_finished_pipeline_execution) instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't have to add em.getTransaction().commit(), as git shows it as a difference, even though it was already there.

We're setting type with pipelineExecution.setTypes in other places, so I decided to follow the same path. I've introduced the new function, addTypes, which allows us to add a new type to the pipeline without replacing it.
image

@blcham blcham force-pushed the improve-pipeline-metadata branch from 0300743 to 644775c Compare November 19, 2025 15:49
@blcham
Copy link
Contributor

blcham commented Nov 19, 2025

@blcham it is needed to review:
image

return getLocation(resourceContextUri);
String location = getLocation(resourceContextUri);

return Paths.get(location).toUri().toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird, why do we need to do this? What is the difference between scriptPath and scriptURI?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting exceptions like this:

ERROR c.c.s.l.AdvancedLoggingProgressListener - Transaction handling error: Not a valid (absolute) IRI: /host_mnt/c/Users/MSattrtand/IntelliJProjects/s-pipes/doc/examples/hello-world/hello-world.sms.ttl [line 21]

The call chain is:

runService -> createFunctionContext -> getFunctionLocation -> getLocation -> OntDocumentManager.doAltURLMapping

According to the latest function Javadoc, it returns "The resolvable location of the alternative copy, if known, or uri otherwise". We know its local path, so we return it, not URI.

context.setScriptUri sets this local path as "_pScriptURI" parameter.

During the pipeline excecution we're getting to this call chain:

moduleExecutionFinished -> writeRawData.

Model, created from our context, contains this triple:

http://onto.fel.cvut.cz/ontologies/s-pipes/query_solution_1763649074917/_pScriptURI http://onto.fel.cvut.cz/ontologies/s-pipes/has_bound_value /host_mnt/c/Users/MSattrtand/IntelliJProjects/s-pipes-editor-ui/deploy/../../s-pipes/doc/examples/hello-world/hello-world.sms.ttl;, so the triple with the absolute path, not URI.

connection.add(
	new StringReader(w.getBuffer().toString()),
	"",
	RDFFormat.N3,
	connection.getValueFactory().createIRI(contextUri.toString()));

Will throw the exception I've described before.

"_pScriptPath" parameter is not used anywhere, since executionContext was introduced, so I'll delete it.

The best solution IMO would be to modify getLocation, so it would return URI, even if it doAltURLMapping returns just a path.

if (inputContext.getScriptUri() != null) {
scriptPath = inputContext.getScriptFile().toString();
} else {
scriptPath = "not defined";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not make sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix execution page not working

2 participants